Make layout support compatible with enhanced pagination #55416

luisherranz · 2023-10-17T13:55:30Z

What?

Partial fix for #55230.

This fix makes the layout support compatible with the enhanced pagination by making sure that the generated class names are stable across paginations, even when the number of rendered posts are different.

Why?

With the current implementation of enhanced pagination, we are still not detecting the CSS corresponding to each block. Therefore, for the enhanced pagination to work correctly, the CSS of the blocks present in the Post Template must be stable on all pages.

The number of posts rendered by the Query block are always the same, except that in the last page, where there can be only a fraction. If any of the blocks rendered on the Post Template use the wp_unique_id function, the ID (which is incremental) will be different than in the previous pages and the class names will vary.

How?

By replacing the usage of wp_unique_id in the layout support (which is used by the Query block) with an implementation that uses IDs that are incremental only for that block. That way, the generated class names are never affected by the number of times wp_unique_id runs.

Testing Instructions

Create multiple posts so you can paginate with the Query block.
Configure the number of posts shown in each page so that on the last page, the number is different.
Add a Group block inside the Post Template (the Group block uses the layout support).
Set the Query block to a grid with at least 2 columns.
Enable the enhanced pagination.
Go to the previous to last page.
Navigate to the last page.
Check that the last page is rendered with 2 columns instead of 1.

Screenshots or screencast

Before:

Screen.Capture.on.2023-10-17.at.15-48-39.mp4

After:

Screen.Capture.on.2023-10-17.at.15-49-20.mp4

github-actions · 2023-10-17T13:58:59Z

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files

❔ lib/block-supports/layout.php

luisherranz · 2023-10-17T15:08:55Z

lib/block-supports/layout.php

+	*/
+	$container_class = gutenberg_incremental_id_per_prefix(
+		'wp-container-' .
+		str_replace( '/', '--', $block['blockName'] ) . '-'


According to the block name requirements:

Note: A block name can only contain lowercase alphanumeric characters, dashes, and at most one forward slash to designate the plugin-unique namespace prefix. It must begin with a letter.

Rather than a manual str_replace would it be worth using sanitize_title for consistency, or is it quicker to use the str_replace?

I'm not sure what's quicker. Sanitize title would replace the slash with a single dash instead of double, but that's not necessarily a bad outcome because we're not using double dashes in these classnames anyway.

I didn't know about sanitize_title. I'll make the change 👍

andrewserong

Thanks for the ping, I like the idea behind this PR in principle! It feels like a clever way to workaround the issue that we don't (yet) have a way to retrieve generated styles associated with a particular post outside of a full page load.

One question about the naming convention: is it a problem that wp-container- + blockName could theoretically be used in other block supports? I was wondering because the position support still uses wp_unique_id( 'wp-container-' ); (here) — so would it be worth including layout somewhere in the prefix, or is the block name enough?

This is testing nicely for me for layout: prior to this PR with enhanced pagination, the two columns in the grid-based layout would not persist after pagination, but with this PR applied, the layout is persisting 👍

One thing I did notice, though, is that with this PR applied, I appear to see the page loaded text visually, after interacting with the pagination buttons. Is that expected? Here:

Without this PR applied, I don't see the Page Loaded. text:

I'll be AFK until next week so might not get to any replies quickly I'm sorry!

tellthemachines

is it a problem that wp-container- + blockName could theoretically be used in other block supports? I was wondering because the position support still uses wp_unique_id( 'wp-container-' ); (here) — so would it be worth including layout somewhere in the prefix, or is the block name enough?

That's a good point! I can't think of any reason not to do that 😄

One thing I did notice, though, is that with this PR applied, I appear to see the page loaded text visually, after interacting with the pagination buttons.

Oh weird, I can't reproduce that locally or on gutenberg.run 🤔

tellthemachines · 2023-10-18T04:26:32Z

lib/block-supports/layout.php

+	*/
+	$container_class = gutenberg_incremental_id_per_prefix(
+		'wp-container-' .
+		str_replace( '/', '--', $block['blockName'] ) . '-'


I'm not sure what's quicker. Sanitize title would replace the slash with a single dash instead of double, but that's not necessarily a bad outcome because we're not using double dashes in these classnames anyway.

andrewserong · 2023-10-18T04:55:41Z

Oh weird, I can't reproduce that locally or on gutenberg.run 🤔

Hrm, very weird! I can't reproduce it via gutenberg.run, either

In case it helps anyone in testing I've copied my markup below — but I'm not sure if it'll be reproducible for others because I can't seem to replicate it in gutenberg.run... 🤔

Test markup for Query loop block

<!-- wp:query {"queryId":9,"query":{"perPage":"10","pages":0,"offset":0,"postType":"post","order":"desc","orderBy":"date","author":"","search":"","exclude":[],"sticky":"","inherit":false},"enhancedPagination":true} -->
<div class="wp-block-query"><!-- wp:post-template {"layout":{"type":"grid","columnCount":3}} -->
<!-- wp:group {"style":{"elements":{"heading":{"color":{"text":"#f44343"}},"link":{"color":{"text":"var:preset|color|primary"}}},"color":{"text":"#4f0808"},"border":{"color":"#929df3","width":"3px","radius":"10px"}},"layout":{"type":"constrained"}} -->
<div class="wp-block-group has-border-color has-text-color has-link-color" style="border-color:#929df3;border-width:3px;border-radius:10px;color:#4f0808"><!-- wp:columns {"verticalAlignment":"center"} -->
<div class="wp-block-columns are-vertically-aligned-center"><!-- wp:column {"verticalAlignment":"center","width":"25%"} -->
<div class="wp-block-column is-vertically-aligned-center" style="flex-basis:25%"><!-- wp:post-featured-image {"isLink":true,"aspectRatio":"auto","style":{"color":{"duotone":["#000097","rgb(255, 217, 217)"]}}} /--></div>
<!-- /wp:column -->

<!-- wp:column {"verticalAlignment":"center","width":"75%"} -->
<div class="wp-block-column is-vertically-aligned-center" style="flex-basis:75%"><!-- wp:heading {"level":6} -->
<h6 class="wp-block-heading">A heading</h6>
<!-- /wp:heading -->

<!-- wp:post-title {"isLink":true} /--></div>
<!-- /wp:column --></div>
<!-- /wp:columns --></div>
<!-- /wp:group -->
<!-- /wp:post-template -->

<!-- wp:query-pagination -->
<!-- wp:query-pagination-previous /-->

<!-- wp:query-pagination-numbers /-->

<!-- wp:query-pagination-next /-->
<!-- /wp:query-pagination --></div>
<!-- /wp:query -->

luisherranz · 2023-10-18T10:03:42Z

would it be worth including layout somewhere in the prefix

Great idea. I'll make that change 🙂

Is something like .wp-container-core-group-layout-1 the best name? Or maybe something more like .wp-layout-support-core-group-1?

I appear to see the page loaded text visually, after interacting with the pagination buttons. Is that expected?

I cannot reproduce it either, even with your markup.

@afercia changed recently the CSS that hides the screen reader div here so maybe that is related. I'm going to rebase this PR. Please let me know if your problem disappears. If it doesn't, could you please check that you site is loading the commons.css file and that that file contains a screen-reader-text class? Thanks!

luisherranz · 2023-10-18T10:05:42Z

would it be worth including layout somewhere in the prefix

Great idea. I'll make that change 🙂

Is something like .wp-container-core-group-layout-1 the best name? Or maybe something more like .wp-layout-support-core-group-1?

I appear to see the page loaded text visually, after interacting with the pagination buttons. Is that expected?

I cannot reproduce it either, even with your markup.

@afercia changed recently the CSS that hides the screen reader div here so maybe that is related. I'm going to rebase this PR. Please let me know if your problem disappears.

If it doesn't, could you please check that you site is loading the commons.css file and that that file contains a screen-reader-text class? Thanks!

tellthemachines

Thanks for iterating on this!

I still can't reproduce the bug @andrewserong mentioned. Code LGTM!

luisherranz · 2023-10-19T08:25:57Z

Thanks @tellthemachines.

As @andrewserong said he is going to be AFK, I'm going to go ahead and merge this. If he can reproduce the issue and we find the root problem, we will fix it in a subsequent PR.

* Make layout supports compatible with enhanced pagination * Use sanitize_title and add `layout` to the class name

SiobhyB · 2023-10-19T09:53:40Z

I've gone ahead to cherry-pick this change for inclusion in 6.4's RC2: 3735244

luisherranz · 2023-10-19T10:40:56Z

Backport:

Make layout supports compatible with enhanced pagination wordpress-develop#5528

* Focus submenu button when clicked (#55198) * Focus element manually when open submenu on click * Try using `tabindex="-1"` * Use `tabindex="-1"` also in body when a submenu is opened * Replace tabindex with event listener * Explain the tabindex on <li> * Don't store the element on hover to restore the focus later * Improve explanations * Add tests to cover webkit frontend menu interactions Safari doesn't place focus on a clicked button as expected. These tests verify that when a submenu chevron button is clicked, focus is correctly placed on that button. It also verifies that the click on the body correctly closes any open submenus, which was failing in Safari. * Focus clicked button on Safari Combining the tabindex -1 on the parent li and focusing the button on Safari, and also checking that the relatedTarget is null inside the handleMenuFocusout seems to contain the focus within the menu to not fire the handleMenuFocusout as often, and still works to click on the body to close the menu. * Added the document.addEventListener body click back in Authored by Luis Herranz <[email protected]>. I'm just re-applying the change. * Remove tab keypresses from webkit menu interaction tests Tab keypreses on webkit playwright are really flakey (or it's something in our code that we haven't isolated) so I've split out the webkit tests to test everything I can without using a tab keypress. * Use body click instead for consistency across environments --------- Co-authored-by: Luis Herranz <[email protected]> Co-authored-by: Jerry Jones <[email protected]> * Make layout support compatible with enhanced pagination (#55416) * Make layout supports compatible with enhanced pagination * Use sanitize_title and add `layout` to the class name * Update default fullscreen icon for lightbox trigger (#55463) * Make duotone compatible with enhanced pagination (#55415) * Patterns: fix capabilities settings for pattern categories (#55379) Co-authored-by: Daniel Richards <[email protected]> * Revert "Patterns: fix capabilities settings for pattern categories (#55379)" This reverts commit 6f83c92. * Image block: wrap images with hrefs in an A tag (#55470) * This commit sees what happens when we wrap the image element in an A tag in the editor. This is to ensure any inherited styles from the link element, such as border color, apply to the image. * Removing duplicate <a href /> wrapper Adding disabled onClick and aria attribute * Image: Improve focus management in lightbox (#55428) * Improve focus management This commit removes the logic to set focus differently based on event.pointerType and instead sets focus on the dialog itself when the lightbox opens, and on the lightbox trigger when the lightbox closes. * Add delay before focusing when closing lightbox * Put focus back on close button when opening lightbox It turns out that placing focus on the modal was causing inconsistent behavior in Safari, so I've put the focus back on the close button when the lightbox opens, which performs predictably. I've also added a tabindex to the image, which prevents the focus ring from erroneously showing when opening the lightbox with a mouse in Chrome and Firefox. * Move focus to the dialog when opening the lightbox. * Fix SVG markup. * Consistent indentation with spaces. * Remove unnecessary tabindex --------- Co-authored-by: Andrea Fercia <[email protected]> * Fix: - Update the title when using enhanced pagination --------- Co-authored-by: Mario Santos <[email protected]> Co-authored-by: Luis Herranz <[email protected]> Co-authored-by: Jerry Jones <[email protected]> Co-authored-by: Artemio Morales <[email protected]> Co-authored-by: Glen Davies <[email protected]> Co-authored-by: Daniel Richards <[email protected]> Co-authored-by: Ramon <[email protected]> Co-authored-by: Andrea Fercia <[email protected]>

getdave · 2024-01-22T15:18:02Z

✅ I updated the PHP Sync Tracking Issue for WP 6.5 to note this PR does not require a backport for WP 6.5.

luisherranz added [Type] Bug An existing feature does not function as intended [Feature] Layout Layout block support, its UI controls, and style output. labels Oct 17, 2023

luisherranz requested review from ramonjd, DAreRodz, tellthemachines, andrewserong and SantosGuillamot October 17, 2023 13:55

luisherranz requested a review from spacedmonkey as a code owner October 17, 2023 13:55

luisherranz self-assigned this Oct 17, 2023

luisherranz mentioned this pull request Oct 17, 2023

Query loop with 2 columns and enhanced pagination causes last page to only have 1 column #55230

Closed

luisherranz commented Oct 17, 2023

View reviewed changes

luisherranz mentioned this pull request Oct 17, 2023

Tracking Issue: More core blocks and new store() API #53740

Closed

21 tasks

andrewserong reviewed Oct 18, 2023

View reviewed changes

tellthemachines reviewed Oct 18, 2023

View reviewed changes

luisherranz added Needs PHP backport Needs PHP backport to Core Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Oct 18, 2023

luisherranz added 2 commits October 18, 2023 12:07

Make layout supports compatible with enhanced pagination

cb812ae

Use sanitize_title and add layout to the class name

872baca

luisherranz force-pushed the fix/make-layout-supports-compatible-with-enhanced-pagination branch from 24b22e5 to 872baca Compare October 18, 2023 10:07

tellthemachines approved these changes Oct 18, 2023

View reviewed changes

luisherranz merged commit 7b8cb73 into trunk Oct 19, 2023
49 checks passed

luisherranz deleted the fix/make-layout-supports-compatible-with-enhanced-pagination branch October 19, 2023 08:26

github-actions bot added this to the Gutenberg 17.0 milestone Oct 19, 2023

SiobhyB pushed a commit that referenced this pull request Oct 19, 2023

Make layout support compatible with enhanced pagination (#55416)

3735244

* Make layout supports compatible with enhanced pagination * Use sanitize_title and add `layout` to the class name

SiobhyB removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 19, 2023

SiobhyB mentioned this pull request Oct 19, 2023

Cherry-picked commits for WordPress 6.4 RC2 #55481

Merged

luisherranz mentioned this pull request Oct 19, 2023

Make layout supports compatible with enhanced pagination WordPress/wordpress-develop#5528

Closed

tellthemachines mentioned this pull request Nov 15, 2023

Add layout classes to legacy Group inner container #56130

Merged

getdave mentioned this pull request Jan 18, 2024

Plugin: Synchronize PHP changes for WordPress 6.5 release #57959

Closed

tellthemachines removed the Needs PHP backport Needs PHP backport to Core label Jan 21, 2024

luisherranz added the Backported to WP Core Pull request that has been successfully merged into WP Core label Jan 22, 2024

tellthemachines mentioned this pull request May 18, 2024

Update child layout selector to match core. #61777

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make layout support compatible with enhanced pagination #55416

Make layout support compatible with enhanced pagination #55416

luisherranz commented Oct 17, 2023

github-actions bot commented Oct 17, 2023

luisherranz Oct 17, 2023

andrewserong Oct 18, 2023 •

edited

Loading

tellthemachines Oct 18, 2023

luisherranz Oct 18, 2023

andrewserong left a comment

tellthemachines left a comment

tellthemachines Oct 18, 2023

andrewserong commented Oct 18, 2023

luisherranz commented Oct 18, 2023

luisherranz commented Oct 18, 2023

tellthemachines left a comment

luisherranz commented Oct 19, 2023

SiobhyB commented Oct 19, 2023

luisherranz commented Oct 19, 2023

getdave commented Jan 22, 2024 •

edited

Loading

Make layout support compatible with enhanced pagination #55416

Make layout support compatible with enhanced pagination #55416

Conversation

luisherranz commented Oct 17, 2023

What?

Why?

How?

Testing Instructions

Screenshots or screencast

github-actions bot commented Oct 17, 2023

luisherranz Oct 17, 2023

Choose a reason for hiding this comment

andrewserong Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

tellthemachines Oct 18, 2023

Choose a reason for hiding this comment

luisherranz Oct 18, 2023

Choose a reason for hiding this comment

andrewserong left a comment

Choose a reason for hiding this comment

tellthemachines left a comment

Choose a reason for hiding this comment

tellthemachines Oct 18, 2023

Choose a reason for hiding this comment

andrewserong commented Oct 18, 2023

luisherranz commented Oct 18, 2023

luisherranz commented Oct 18, 2023

tellthemachines left a comment

Choose a reason for hiding this comment

luisherranz commented Oct 19, 2023

SiobhyB commented Oct 19, 2023

luisherranz commented Oct 19, 2023

getdave commented Jan 22, 2024 • edited Loading

andrewserong Oct 18, 2023 •

edited

Loading

getdave commented Jan 22, 2024 •

edited

Loading